outlier: tooling for success rate ejection #618

junr03 · 2017-03-24T00:52:20Z

No description provided.

mattklein123 · 2017-03-24T03:13:50Z

include/envoy/upstream/upstream.h

+   *         -1 means that the host did not have enough request volume to calculate success rate
+   *         or the cluster did not have enough hosts to run through success rate outlier ejection.
+   */
+  virtual double successRate() const PURE;


Both of these should come directly out of the outlier detector sink via here: https://github.com/lyft/envoy/blob/ce8eb8634f0c6aa24485e17fbd563ecc18d73982/include/envoy/upstream/host_description.h#L51

Don't add to host object.

RomanDzhabarov · 2017-03-27T19:58:32Z

docs/operations/admin.rst

@@ -35,6 +35,7 @@ modify different aspects of the server.
    weight, Integer, Load balancing weight (1-100)
    zone, String, Service zone
    canary, Boolean, Whether the host is a canary
+    success_rate, Double, Request success rate (0-100). -1 if there was not enough request volume to calculate it


can you link to the info about "not enough request volume"?

ccaraman · 2017-03-27T22:12:12Z

include/envoy/upstream/outlier_detection.h

@@ -65,8 +78,9 @@ class EventLogger {
   * Log an ejection event.
   * @param host supplies the host that generated the event.
   * @param type supplies the type of the event.
+   * @param enforced is true if the ejection took place, false if only logging took place.


run on sentence.

ccaraman · 2017-03-27T22:17:44Z

source/common/upstream/outlier_detection_impl.cc

@@ -266,6 +271,9 @@ double Utility::successRateEjectionThreshold(
  variance /= valid_success_rate_hosts.size();
  double stdev = std::sqrt(variance);

+  // Set the Detector's member variable


nit: full stop.

RomanDzhabarov · 2017-03-27T22:18:13Z

include/envoy/upstream/outlier_detection.h

@@ -48,6 +48,19 @@ class DetectorHostSink {
   * @return the last time this host was unejected, if the host has been unejected previously.
   */
  virtual const Optional<SystemTime>& lastUnejectionTime() PURE;
+
+  /**
+   * @return the success rate of the host in the last calculated interval, in the range -1-100.


-1-100 is a bit misleading, as it's [0;100] with -1 as a separate value. I like the way you cover this in docs, maybe do the same here.

mattklein123 · 2017-03-27T22:15:04Z

docs/operations/admin.rst

@@ -35,6 +35,9 @@ modify different aspects of the server.
    weight, Integer, Load balancing weight (1-100)
    zone, String, Service zone
    canary, Boolean, Whether the host is a canary
+    success_rate, Double, "Request success rate (0-100). -1 if there was not enough


Link to some docs here (or write it here) that describe what interval this is over.

mattklein123 · 2017-03-27T22:15:47Z

include/envoy/upstream/outlier_detection.h

+   * Set the success rate of the host.
+   * @param new_success_rate the new success rate calculated for the host in the last interval.
+   */
+  virtual void successRate(double new_success_rate) PURE;


This doesn't need to be in public interface.

mattklein123 · 2017-03-27T22:17:41Z

source/common/upstream/outlier_detection_impl.cc

@@ -241,7 +245,8 @@ void DetectorImpl::onConsecutive5xxWorker(HostSharedPtr host) {
 const double Utility::SUCCESS_RATE_STDEV_FACTOR = 1.9;

 double Utility::successRateEjectionThreshold(
-    double success_rate_sum, const std::vector<HostSuccessRatePair>& valid_success_rate_hosts) {
+    double success_rate_sum, const std::vector<HostSuccessRatePair>& valid_success_rate_hosts,
+    double& success_rate_average) {


Instead of passing an out param, can you return a struct. It's easier to grok what is going on.

mattklein123 · 2017-03-27T22:20:36Z

source/common/upstream/outlier_detection_impl.cc

@@ -330,7 +348,8 @@ void DetectorImpl::runCallbacks(HostSharedPtr host) {
  }
 }

-void EventLoggerImpl::logEject(HostDescriptionConstSharedPtr host, EjectionType type) {
+void EventLoggerImpl::logEject(HostDescriptionConstSharedPtr host, EjectionType type,


Wouldn't it be useful to log some of the SR stats here also to see the SR, the cutoff, etc.?

mattklein123 · 2017-03-27T22:22:03Z

include/envoy/upstream/upstream.h

@@ -300,6 +301,8 @@ class Cluster : public virtual HostSet {
   */
  virtual ClusterInfoConstSharedPtr info() const PURE;

+  virtual Outlier::DetectorSharedPtr outlierDetector() const PURE;


comment, this should return a pointer to a const detector.

RomanDzhabarov · 2017-03-27T22:25:22Z

source/common/upstream/outlier_detection_impl.h

@@ -246,7 +255,8 @@ class Utility {
   */
  static double
  successRateEjectionThreshold(double success_rate_sum,
-                               const std::vector<HostSuccessRatePair>& valid_success_rate_hosts);
+                               const std::vector<HostSuccessRatePair>& valid_success_rate_hosts,


update comment

RomanDzhabarov · 2017-03-27T22:27:35Z

source/server/http/admin.cc

+                               Upstream::Outlier::DetectorSharedPtr outlier_detector,
+                               Buffer::Instance& response) {
+  if (outlier_detector) {
+    response.add(fmt::format("{}::outlier::success_rate_average::{}", cluster_name,


put all these into docs?

mattklein123 · 2017-03-29T19:35:45Z

docs/intro/arch_overview/outlier.rst

-  If ``action`` is ``eject``, species the type of ejection that took place. Currently this can
-  only be ``5xx``.
+  If ``action`` is ``eject``, specifies the type of ejection that took place. Currently this can be:
+  ``5xx``, and ``SuccessRate``.


No comma after 5xx

I thought you were a supporter of the oxford comma ;)

Yes, when there are 3 or more things, not 2.

(Comma still needs to be deleted)

mattklein123 · 2017-03-29T19:37:13Z

docs/intro/arch_overview/outlier.rst

+
+host_success_rate
+  If ``action`` is ``eject``, and ``type`` is ``SuccessRate``, specifies the host's success rate
+  at the time of the ejection event on a ``0-100`` range. A ``-1`` value means that a success


-1 can only happen if type is 5xx, right? This is kind of confusing. Can we just not write out host_succes_rate in the case of 5xx? Same for all the ones below.

-1 can also happen if the type is success rate, i.e if there is no enough request volume. However, you are right it would never happen in the logs. Because, if a host is ejected it is because there was enough data to do that. -1 would happen in the admin /clusters endpoint, not here.

mattklein123 · 2017-03-29T19:39:15Z

docs/operations/admin.rst

+      zone, String, Service zone
+      canary, Boolean, Whether the host is a canary
+      success_rate, Double, "Request success rate (0-100). -1 if there was not enough
+      :ref:`request volume<config_cluster_manager_cluster_outlier_detection_success_rate_request_volume>`


Did you actually look at these docs rendered? This is almost definitely not correct and will look broken.

Yeah, I rendered them and they look correct

mattklein123 · 2017-03-29T19:40:55Z

include/envoy/upstream/upstream.h

+   * @return a shared pointer to the cluster's outlier detector. If an outlier detector has not been
+   *         installed, returns a nullptr.
+   */
+  virtual const Outlier::DetectorSharedPtr outlierDetector() const PURE;


return const shared pointer has no meaning. You need to return a pointer to a const Detector.

Ah I see this is a const pointer not a pointer to a const object. What we want is to return a pointer to a const object that way we can't change the object through the pointer.

mattklein123 · 2017-03-29T19:41:48Z

source/common/upstream/outlier_detection_impl.cc

+                             host->cluster().name(), host->address()->asString(),
+                             typeToString(type), host->outlierDetector().numEjections(), enforced,
+                             host->outlierDetector().successRate(), detector.successRateAverage(),
+                             detector.successRateEjectionThreshold()));


add break here for next person that comes along.

mattklein123 · 2017-03-29T19:42:34Z

source/common/upstream/outlier_detection_impl.h

@@ -237,14 +247,21 @@ class EventLoggerImpl : public EventLogger {
 */
 class Utility {
 public:
+  struct EjectionPair {
+    EjectionPair(double success_rate_average, double ejection_threshold)


You don't need constructor here. You can just do aggregate init ( {...} )

mattklein123 · 2017-03-29T19:42:43Z

source/common/upstream/outlier_detection_impl.h

  /**
   * This function returns the success rate threshold for success rate outlier detection. If a
   * host's success rate is under this threshold the host is an outlier.
   * @param success_rate_sum is the sum of the data in the success_rate_data vector.
-   * @param success_rate_data is the vector containing the individual success rate data points.
+   * @param valid_success_rate_hosts is the vector containing the individual success rate data
+   * points.
   * @return the success rate threshold.


comment wrong

mattklein123 · 2017-03-29T23:46:33Z

Looks good other than comma still in docs. Do you have 100% coverage?

ccaraman · 2017-03-30T19:48:29Z

docs/intro/arch_overview/outlier.rst

+    "host_success_rate": "...",
+    "cluster_success_rate_average": "...",
+    "cluster_success_rate_ejection_threshold": "..."
+


nit: new line

ccaraman · 2017-03-30T20:46:36Z

docs/intro/arch_overview/outlier.rst

-  If ``action`` is ``eject``, species the type of ejection that took place. Currently this can
-  only be ``5xx``.
+  If ``action`` is ``eject``, specifies the type of ejection that took place. Currently this can be:
+  ``5xx`` and ``SuccessRate``.


The second sentence doesn't make sense. Can you rewrite it? Also colon after be isn't needed.

ccaraman · 2017-03-30T23:38:06Z

docs/intro/arch_overview/outlier.rst

  }

 time
  The time that the event took place.

 secs_since_last_action
  The time in seconds since the last action (either an ejection or unejection)
-  took place. This time will be -1 for the first ejection given there is no
+  took place. This time will be ``-1`` for the first ejection given there is no


change this time will be to the value will be

ccaraman · 2017-03-30T23:51:01Z

docs/intro/arch_overview/outlier.rst

+  reason and then re-added).
+
+enforced
+  If ``action`` is ``eject``, specifies if the ejection was actually performed (``true``), or


this sentence is too long and confusing. Please break it up.

ccaraman · 2017-03-30T23:59:44Z

source/common/upstream/outlier_detection_impl.h

@@ -95,19 +96,22 @@ class DetectorHostSinkImpl : public DetectorHostSink {
      : detector_(detector), host_(host) {
    // Point the success_rate_accumulator_bucket_ pointer to a bucket.
    updateCurrentSuccessRateBucket();
+    success_rate_ = -1;


move to initializer list

ccaraman · 2017-03-31T00:01:06Z

source/common/upstream/outlier_detection_impl.h

  /**
-   * This function returns the success rate threshold for success rate outlier detection. If a
-   * host's success rate is under this threshold the host is an outlier.
+   * This function returns the an EjectionPair for success rate outlier detection. The pair contains


extra the after returns

ccaraman · 2017-03-31T00:01:30Z

source/common/upstream/outlier_detection_impl.h

-   * host's success rate is under this threshold the host is an outlier.
+   * This function returns the an EjectionPair for success rate outlier detection. The pair contains
+   * the average success rate of all valid hosts in the cluster, and the ejection threshold.
+   * If a host's success rate is under this threshold the host is an outlier


nit full stop at the end.

comma after the first sentence

ccaraman · 2017-03-31T00:01:38Z

source/common/upstream/outlier_detection_impl.h

-   * @param success_rate_data is the vector containing the individual success rate data points.
-   * @return the success rate threshold.
+   * @param valid_success_rate_hosts is the vector containing the individual success rate data
+   * points.


nit formating.

ccaraman · 2017-03-31T00:03:07Z

source/common/upstream/outlier_detection_impl.cc

+
+    // Need to update the writer bucket to keep the data valid.
+    host.second->updateCurrentSuccessRateBucket();
+    // Refresh host success rate stat for the /clusters endpoint. If there is a new valid value it


comma after the first sentence in the if sentence.

ccaraman · 2017-03-31T00:04:15Z

source/common/upstream/outlier_detection_impl.h

-   * This function returns the success rate threshold for success rate outlier detection. If a
-   * host's success rate is under this threshold the host is an outlier.
+   * This function returns the an EjectionPair for success rate outlier detection. The pair contains
+   * the average success rate of all valid hosts in the cluster, and the ejection threshold.


no comma needed before the and

ccaraman · 2017-03-31T00:06:23Z

source/common/upstream/outlier_detection_impl.cc

@@ -277,6 +281,10 @@ void DetectorImpl::processSuccessRateEjections() {
  std::vector<HostSuccessRatePair> valid_success_rate_hosts;
  double success_rate_sum = 0;

+  // Reset the Detector's success rate mean and stdev


nit full stop

ccaraman · 2017-03-31T00:10:28Z

source/common/upstream/outlier_detection_impl.h

@@ -212,6 +219,8 @@ class DetectorImpl : public Detector, public std::enable_shared_from_this<Detect
  std::list<ChangeStateCb> callbacks_;
  std::unordered_map<HostSharedPtr, DetectorHostSinkImpl*> host_sinks_;
  EventLoggerSharedPtr event_logger_;
+  double success_rate_average_;


do we need default values for these?

including success_rate_ejection_threshold_

ccaraman · 2017-03-31T17:18:01Z

include/envoy/upstream/outlier_detection.h

+  virtual void logUneject(HostDescriptionConstSharedPtr host) PURE;
+};
+
+typedef std::shared_ptr<EventLogger> EventLoggerSharedPtr;


nit: new line after type def

ccaraman

Looks good after few new line nits

* Integrate with new mixer control library. * Update to use splitted control lib. * remove quota config files. * Update per comment

Signed-off-by: Mandar U Jog <[email protected]>

Fixes envoyproxy/envoy-mobile#617, which resulted in duplicate symbols from protobuf being present in the final Envoy Mobile binary. The [root cause](bazelbuild/bazel#10484) is being [patched in Bazel](bazelbuild/bazel#10493). In the meantime, we can revert these two commits in protobuf ([first](protocolbuffers/protobuf@7b28278), [second](protocolbuffers/protobuf@a03d332)) and apply the diff as a patch until we're able to update Bazel with the fix. More context on the protobuf issue is available [here](protocolbuffers/protobuf#7046). Notes: - This should be reverted after updating Bazel with the fix - The version of protobuf specified in the `http_archive` should be in lockstep with upstream Envoy's version - Upstream Envoy's `@envoy//bazel:protobuf.patch` patch is also included here Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>

Jose Nino added 2 commits March 23, 2017 17:51

outlier: tooling for success rate ejection

41f5a06

docs for admin port

ce8eb86

mattklein123 reviewed Mar 24, 2017

View reviewed changes

Jose Nino added 2 commits March 27, 2017 11:19

update

edaf832

update

6ddc858

RomanDzhabarov reviewed Mar 27, 2017

View reviewed changes

link

99df9b4

ccaraman reviewed Mar 27, 2017

View reviewed changes

RomanDzhabarov reviewed Mar 27, 2017

View reviewed changes

mattklein123 reviewed Mar 27, 2017

View reviewed changes

RomanDzhabarov reviewed Mar 27, 2017

View reviewed changes

Jose Nino added 3 commits March 28, 2017 13:54

Merge branch 'master' into outlier-sr-tooling

1f3ff02

update

5953641

docs

24cedc8

mattklein123 reviewed Mar 29, 2017

View reviewed changes

Jose Nino added 2 commits March 29, 2017 15:34

update

84a8f97

return

b1e5845

coverage

3825f62

junr03 force-pushed the outlier-sr-tooling branch from 363fc10 to 3825f62 Compare March 30, 2017 18:49

ccaraman reviewed Mar 30, 2017

View reviewed changes

nit

7d3f240

ccaraman reviewed Mar 30, 2017

View reviewed changes

sentence

08f1fcd

ccaraman reviewed Mar 30, 2017

View reviewed changes

ccaraman reviewed Mar 31, 2017

View reviewed changes

Jose Nino added 3 commits March 30, 2017 18:14

Merge branch 'master' into outlier-sr-tooling

a401824

comments

0bd0487

format

c103237

ccaraman reviewed Mar 31, 2017

View reviewed changes

ccaraman approved these changes Mar 31, 2017

View reviewed changes

format

eaedc71

junr03 merged commit 0f5d9f5 into master Mar 31, 2017

junr03 deleted the outlier-sr-tooling branch March 31, 2017 18:25

vijayendrabvs pushed a commit to vijayendrabvs/envoy that referenced this pull request Apr 4, 2017

outlier: tooling for success rate ejection (envoyproxy#618)

edf7e36

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

Integrate with new mixer control library. (envoyproxy#618)

54dd33e

* Integrate with new mixer control library. * Update to use splitted control lib. * remove quota config files. * Update per comment

jplevyak pushed a commit to jplevyak/envoy that referenced this pull request Aug 17, 2020

Update to antlr4 472 for Cel (envoyproxy#618)

6e5baef

Signed-off-by: Mandar U Jog <[email protected]>

outlier: tooling for success rate ejection #618

outlier: tooling for success rate ejection #618

Conversation

junr03 commented Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccaraman left a comment

Choose a reason for hiding this comment

junr03 commented Mar 24, 2017 •

edited

Loading